[Sema] Don't drop weak_import from a declaration that follows a declaration directly contained in a linkage-specification#85886
Conversation
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Akira Hatanaka (ahatanak) ChangesOnly drop it if the declaration follows a definition. I believe this is what 33e0226 was trying to do. rdar://61865848 Full diff: https://github.com/llvm/llvm-project/pull/85886.diff 2 Files Affected:
diff --git a/clang/lib/Sema/SemaDecl.cpp b/clang/lib/Sema/SemaDecl.cpp
index 5850cd0ab6b9aa..2e45f1191273a4 100644
--- a/clang/lib/Sema/SemaDecl.cpp
+++ b/clang/lib/Sema/SemaDecl.cpp
@@ -4613,8 +4613,7 @@ void Sema::MergeVarDecl(VarDecl *New, LookupResult &Previous) {
mergeDeclAttributes(New, Old);
// Warn if an already-declared variable is made a weak_import in a subsequent
// declaration
- if (New->hasAttr<WeakImportAttr>() &&
- Old->getStorageClass() == SC_None &&
+ if (New->hasAttr<WeakImportAttr>() && Old->isThisDeclarationADefinition() &&
!Old->hasAttr<WeakImportAttr>()) {
Diag(New->getLocation(), diag::warn_weak_import) << New->getDeclName();
Diag(Old->getLocation(), diag::note_previous_declaration);
diff --git a/clang/test/SemaCXX/attr-weak.cpp b/clang/test/SemaCXX/attr-weak.cpp
index f065bfd9483f8a..a2c5fd4abd35f6 100644
--- a/clang/test/SemaCXX/attr-weak.cpp
+++ b/clang/test/SemaCXX/attr-weak.cpp
@@ -55,3 +55,10 @@ constexpr bool weak_method_is_non_null = &WithWeakMember::weak_method != nullptr
// virtual member function is present.
constexpr bool virtual_weak_method_is_non_null = &WithWeakMember::virtual_weak_method != nullptr; // expected-error {{must be initialized by a constant expression}}
// expected-note@-1 {{comparison against pointer to weak member 'WithWeakMember::virtual_weak_method' can only be performed at runtime}}
+
+// Check that no warnings are emitted.
+extern "C" int g0;
+extern int g0 __attribute__((weak_import));
+
+extern "C" int g1 = 0; // expected-note {{previous definition is here}}
+extern int g1 __attribute__((weak_import)); // expected-warning {{attribute declaration must precede definition}}
|
declaration directly contained in a linkage-specification Only drop it if the declaration follows a definition. I believe this is what 33e0226 was trying to do. rdar://61865848
ahatanak
force-pushed
the
weak-import-decl
branch
from
1ccbc2e to
d39667c
Compare
There was a problem hiding this comment.
I think you're right about the intended logic being to check for a definition, especially given the wording of the warning. IIRC, we didn't have high-level functions for checking some of these conditions at the time, so looking directly at the storage-class specifier would have been common.
With that said, I think you need to check if a definition exists at all and not just whether the last declaration is that definition.
Note that |
|
The updated patch checks that the decl isn't |
rjmccall
left a comment
rjmccall
left a comment
There was a problem hiding this comment.
LGTM. The (existing) diagnostic wording does seem a little awkward, so if you want to put effort into cleaning that up, it'd be welcome, but I won't hold up this fix.
…n't seen (llvm#85886) I believe this is what the original commit (33e0226) was trying to do. This fixes a bug where clang removes the attribute from a declaration that follows a declaration directly contained in a linkage-specification. rdar://61865848 (cherry picked from commit 884772f) Conflicts: clang/test/Sema/attr-weak.c
…n't seen (llvm#85886) I believe this is what the original commit (33e0226) was trying to do. This fixes a bug where clang removes the attribute from a declaration that follows a declaration directly contained in a linkage-specification. rdar://61865848 (cherry picked from commit 884772f) Conflicts: clang/test/Sema/attr-weak.c
3 participants
Only drop it if the declaration follows a definition. I believe this is what 33e0226 was trying to do.
rdar://61865848